Add canonical#3008
Conversation
| end | ||
|
|
||
| # Workaround: both `is_canonical` and `canonicalize!` would be slow otherwise | ||
| canonical(f::MOI.ScalarNonlinearFunction) = f |
There was a problem hiding this comment.
Ask Claude why they would add this method when there is the one below 😆
There was a problem hiding this comment.
I did it pre-Claude actually 😅 this line is just a temporary hack to isolate things in profiling
| # end | ||
| # end | ||
| # return true | ||
| #end |
There was a problem hiding this comment.
Delete this code? It isn't correct regardless, because the function is canonical but it returns a ::Bool instead of the canonicalised function.
There was a problem hiding this comment.
Why not change the is_canonical function? With this change it can be that is_canonical(canonical(f)) == false
There was a problem hiding this comment.
What's the underlying problem? That we walk the expression graph?
There was a problem hiding this comment.
I wrote this in July 2025, I don't remember much, I will need to look at it again but let's first focus on merging jump-dev/JuMP.jl#4032, then we'll look at the MOI level
| # JuMP-side of profiling in https://github.com/jump-dev/JuMP.jl/pull/4032 | ||
| # We still need to figure out what's the right fix here anyway | ||
| #function canonical(f::MOI.ScalarNonlinearFunction) | ||
| # cache = Dict{MOI.AbstractScalarFunction,MOI.AbstractScalarFunction}() |
There was a problem hiding this comment.
The goal of this was to write a version with this cache but I first want to check that this is really necessary
2 participants
Extracted from #2803
I'm not too convinced by the current approach.
For
canonical, I think we need to implementcanonicalwith a cache that checks that it creates aliases when it sees a second time an alias to the same sub-expression.